Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Use ternary operator for conditional assignments #394

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

webmaster128
Copy link
Collaborator

@webmaster128 webmaster128 commented Nov 16, 2021

That's the second step initially proposed in #371. In cases where the input value needs to be transformed, we cannot use nullish coalescing or anything new and fancy.

This improves the code output in two ways:

  • Less code (that ultimatly reduces the app bundle size for the end user)
  • Exactly one assignment per field which I find easier to reason about

(isTimestamp(field) && (options.useDate === DateOption.DATE || options.useDate === DateOption.STRING)) ||
isValueType(ctx, field)
) {
} else if (readSnippet(`x`).toCodeString() == 'x') {
Copy link
Collaborator Author

@webmaster128 webmaster128 Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This somehwat unrelated change (first commit) is worth having a 👀. It did not change the code output at all which I guess means it is correct.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, that's a neat way of doing this.

Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

(isTimestamp(field) && (options.useDate === DateOption.DATE || options.useDate === DateOption.STRING)) ||
isValueType(ctx, field)
) {
} else if (readSnippet(`x`).toCodeString() == 'x') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, that's a neat way of doing this.

@webmaster128
Copy link
Collaborator Author

Thanks!

@webmaster128 webmaster128 merged commit d84c084 into stephenh:main Nov 16, 2021
@webmaster128 webmaster128 deleted the improve-code-output branch November 16, 2021 15:57
stephenh pushed a commit that referenced this pull request Nov 16, 2021
# [1.87.0](v1.86.0...v1.87.0) (2021-11-16)

### Features

* Use ternary operator for conditional assignments ([#394](#394)) ([d84c084](d84c084))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.87.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants